Allow OAuth client provider to implement token endpoint authentication methods#531
Allow OAuth client provider to implement token endpoint authentication methods#531jaredhanson wants to merge 7 commits intomodelcontextprotocol:mainfrom
Conversation
… during authorization code exchange.
… during refresh token exchange.
pcarleton
left a comment
pcarleton
left a comment
There was a problem hiding this comment.
Hey thanks for this, the direction overall looks good.
I'd like to know more about whether those functions are heavily used / how bad of a breaking change this would be before merging. I agree on appearance that they're only used there, but I know we at least use them in Inspector for the debugger (probably not a super common use case), so want to look into it a bit more.
Alternatively, sticking it in as optional seems like a fine option.
… refreshAuthorization to maintain compatibility.
|
Thanks @pcarleton - I modified the PR to make |
|
Hi @jaredhanson, thanks for sending this out! Main change is I've passed the (renamed) addClientAuthentication callback explicitly to refreshAuthorization & exchangeAuthorization, to avoid the awkwardness of them accepting a provider + other redundant parameters (codeVerifier, redirectUri). |
4 participants
Adds an optional
authToTokenEndpointfunction to theOAuthClientProviderinterface. This allows client providers implement other token endpoint authentication methods such asprivate_key_jwtthat are more secure thanclient_secret_postandnone, which are implemented by default currently.Motivation and Context
Allows implementation of other methods for authenticating to the token endpoint.
How Has This Been Tested?
This is being tested in a real agent that I'm currently developing, which is using key pairs for authentication.
Breaking Changes
This adds a required
providerparameter toexchangeAuthorizationandrefreshAuthorizationwhich are exported from@modelcontextprotocol/sdk/client/auth. In that sense it is a breaking change. However, these functions seem to be used only internally to this package, which limits the impact.Types of changes
NOTE: This could be considered breaking, per description above, but it seems low probability at this point.
Checklist